Update "focus mode" to consistently use the Document Bar's Back button #58528
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
noisysocks
added
[Type] Enhancement
noisysocks
requested review from
youknowriad,
SaxonF,
glendaviesnz and
andrewserong
|
Size Change: -9.68 kB (-1%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
Just an FYI, I don't think it has a big impact on this PR, but we are renaming the editor settings that were added to handle the focus mode navigation #58416 |
andrewserong
reviewed
Feb 1, 2024
There was a problem hiding this comment.
Very cool! Nice idea giving more consistency to the Back button, I know @tellthemachines has flagged that previously as being an issue 👍
Visually it's looking good to me, but I ran into a couple of issues with the Back button in my test site when using the "Always open list view" preference. The problem appears to exist on trunk, too, and I think I got to the bottom of what's causing it. I've put up a potential fix up over in #58533
The problem was, with the list view open, I got editor errors in the following situations:
- In the post editor, if I go the edit the template, and then click the back button
- In the site editor, if I go to edit a page, then click to edit the page's template, and then click the back button
Early returning the ListViewLeaf component if a block isn't available seems to do the trick — I suspect the problem is that if we hit the Back button with the list view open, there's a moment during unmount or thereabouts where the component is trying to render an existing list view row but the block no longer exists because we've switched rendering modes.
|
Love the new more relaxed color, the black has been kind of in-your-face jarring. |
youknowriad
reviewed
Feb 1, 2024
| const goBack = useMemo( () => { | ||
| const isFocusMode = | ||
| location.params.focusMode || | ||
| FOCUSABLE_ENTITIES.includes( location.params.postType ); |
There was a problem hiding this comment.
I think it's possible that the url has postType but no postId in which case, we shouldn't have a goBack.
There was a problem hiding this comment.
Also, I believe we should be checking the "previous" location, not the current one.
There was a problem hiding this comment.
I'm reusing the existing logic we have for showing the back button:
What do you mean by checking the previous location?
There was a problem hiding this comment.
Taking a stab at what you mean but also sharing my idle thoughts 🙂
It would be a nicer flow if we only showed the Back button when you get there by e.g. selecting a pattern block and clicking Edit and not when you get there using the W menu. I'll see if it's possible. I don't think we currently keep track of the entity that you were previously editing.
There was a problem hiding this comment.
It would be a nicer flow if we only showed the Back button when you get there by e.g. selecting a pattern block and clicking Edit and not when you get there using the W menu. I'll see if it's possible. I don't think we currently keep track of the entity that you were previously editing.
This is basically what I'm saying: the "back button" should only be visible if the previous page is a non focused entity (so postId and postType) defined but also different than the current entity.
There was a problem hiding this comment.
Let's handle this in a follow-up. I think we'd need to make changes to @wordpress/router which deserve some consideration. Right now the logic controlling when the back button appears is the same as it is in trunk, just the button is in a different place.
There was a problem hiding this comment.
This was simpler than I thought: #58710 TIL about usePrevious 😅
youknowriad
reviewed
Feb 1, 2024
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core SVNIf you're a Core Committer, use this list when committing to GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Thank you "fixing" this! It was much needed. I'm curious to see what sort of feedback we get during beta. We might need to bump up the visibility of the back button but I'd consider that a separate follow up issue. The consistency introduced here is great. I personally like the lighter colour for the canvas background. In future we can do some fun things like pick a light/dark colour depending on the site and/or change when in light/dark mode. |
|
I'm seeing a bug when testing this one:
|
|
@noisysocks is the error that Riad ran into the one that we fixed in #58533? This is what I'm seeing following the above steps (with the list view open). Hopefully it should be resolved with a rebase? 
|
andrewserong
approved these changes
Feb 6, 2024
There was a problem hiding this comment.
Code change looks good, the light colour looks nice to me, and the behaviour appears to be identical to the previous BackButton component, making this a nice iterative change IMO 👍
Looks like the error that Riad encountered was likely the one fixed by #58533 as I can't reproduce the issue once I've rebased this PR locally.
LGTM! ✨
|
Awesome, thanks for looking into that @andrewserong! |
This was referenced Feb 7, 2024
This was referenced Feb 12, 2024
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
#57700 was recently merged which made the post editor and site editor consistently use "focus mode" (where you get a thick black border around the editor canvas) when editing a template part, a pattern, and a page's template.
This is good, and it even addresses Item 3 in #55025, but there's still a lot of inconsistency. In some places we use the Back button that appears within the black border and in other places we use the Back button that appears in the Document Bar at the top of the screen.
This PR:
Refactorsedit: I split this out into DocumentBar: Simplify component, use framer for animation #58656.DocumentBarslightly and changes it to use Framer motion instead of CSS animation to animate the Back button. This reduces the code complexity of the component.Why?
Consistency! And it addresses Item 2 of #55025.
How?
@wordpress/editoralready supports agoBackprop which, if provided, will render the Back button inDocumentBar. This lets us deleteBackButtonfrom@wordpress/edit-siteand move the logic that's there to auseGoBackhook instead.Testing Instructions
Check all of the places that we use "focus mode" in both editors:
Screenshots or screencast
Kapture.2024-02-01.at.12.21.04.mp4